-
Notifications
You must be signed in to change notification settings - Fork 315
Add Config Inversion Linter for Config Definitions #9849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
🎯 Code Coverage 🔗 Commit SHA: 586c53e | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 58 metrics, 7 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.55.0-SNAPSHOT~586c53ec38, baseline=1.56.0-SNAPSHOT~46649f7687
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.11 s) : 0, 1110305
Total [baseline] (10.747 s) : 0, 10746982
Agent [candidate] (1.117 s) : 0, 1116797
Total [candidate] (10.819 s) : 0, 10819297
section appsec
Agent [baseline] (1.28 s) : 0, 1280276
Total [baseline] (11.14 s) : 0, 11140328
Agent [candidate] (1.278 s) : 0, 1278293
Total [candidate] (11.17 s) : 0, 11170211
section iast
Agent [baseline] (1.242 s) : 0, 1241735
Total [baseline] (11.248 s) : 0, 11247644
Agent [candidate] (1.244 s) : 0, 1243615
Total [candidate] (11.299 s) : 0, 11298701
section profiling
Agent [baseline] (1.236 s) : 0, 1236333
Total [baseline] (11.106 s) : 0, 11106208
Agent [candidate] (1.236 s) : 0, 1235759
Total [candidate] (11.089 s) : 0, 11089258
gantt
title petclinic - break down per module: candidate=1.55.0-SNAPSHOT~586c53ec38, baseline=1.56.0-SNAPSHOT~46649f7687
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.463 ms) : 0, 1463
crashtracking [candidate] (1.469 ms) : 0, 1469
BytebuddyAgent [baseline] (710.603 ms) : 0, 710603
BytebuddyAgent [candidate] (713.622 ms) : 0, 713622
GlobalTracer [baseline] (249.538 ms) : 0, 249538
GlobalTracer [candidate] (251.062 ms) : 0, 251062
AppSec [baseline] (32.439 ms) : 0, 32439
AppSec [candidate] (32.9 ms) : 0, 32900
Debugger [baseline] (69.02 ms) : 0, 69020
Debugger [candidate] (70.032 ms) : 0, 70032
Remote Config [baseline] (626.926 µs) : 0, 627
Remote Config [candidate] (658.778 µs) : 0, 659
Telemetry [baseline] (8.112 ms) : 0, 8112
Telemetry [candidate] (8.386 ms) : 0, 8386
Flare Poller [baseline] (3.648 ms) : 0, 3648
Flare Poller [candidate] (3.786 ms) : 0, 3786
section appsec
crashtracking [baseline] (1.442 ms) : 0, 1442
crashtracking [candidate] (1.473 ms) : 0, 1473
BytebuddyAgent [baseline] (729.422 ms) : 0, 729422
BytebuddyAgent [candidate] (728.985 ms) : 0, 728985
GlobalTracer [baseline] (240.772 ms) : 0, 240772
GlobalTracer [candidate] (240.661 ms) : 0, 240661
IAST [baseline] (24.67 ms) : 0, 24670
IAST [candidate] (24.864 ms) : 0, 24864
AppSec [baseline] (174.347 ms) : 0, 174347
AppSec [candidate] (173.675 ms) : 0, 173675
Debugger [baseline] (61.85 ms) : 0, 61850
Debugger [candidate] (60.978 ms) : 0, 60978
Remote Config [baseline] (677.398 µs) : 0, 677
Remote Config [candidate] (651.893 µs) : 0, 652
Telemetry [baseline] (8.475 ms) : 0, 8475
Telemetry [candidate] (8.416 ms) : 0, 8416
Flare Poller [baseline] (3.943 ms) : 0, 3943
Flare Poller [candidate] (3.891 ms) : 0, 3891
section iast
crashtracking [baseline] (1.458 ms) : 0, 1458
crashtracking [candidate] (1.452 ms) : 0, 1452
BytebuddyAgent [baseline] (828.669 ms) : 0, 828669
BytebuddyAgent [candidate] (830.261 ms) : 0, 830261
GlobalTracer [baseline] (238.086 ms) : 0, 238086
GlobalTracer [candidate] (238.547 ms) : 0, 238547
IAST [baseline] (29.998 ms) : 0, 29998
IAST [candidate] (30.903 ms) : 0, 30903
AppSec [baseline] (31.285 ms) : 0, 31285
AppSec [candidate] (30.486 ms) : 0, 30486
Debugger [baseline] (65.858 ms) : 0, 65858
Debugger [candidate] (65.592 ms) : 0, 65592
Remote Config [baseline] (537.961 µs) : 0, 538
Remote Config [candidate] (541.423 µs) : 0, 541
Telemetry [baseline] (7.66 ms) : 0, 7660
Telemetry [candidate] (7.668 ms) : 0, 7668
Flare Poller [baseline] (3.526 ms) : 0, 3526
Flare Poller [candidate] (3.526 ms) : 0, 3526
section profiling
crashtracking [baseline] (1.455 ms) : 0, 1455
crashtracking [candidate] (1.442 ms) : 0, 1442
BytebuddyAgent [baseline] (733.663 ms) : 0, 733663
BytebuddyAgent [candidate] (731.855 ms) : 0, 731855
GlobalTracer [baseline] (221.552 ms) : 0, 221552
GlobalTracer [candidate] (222.912 ms) : 0, 222912
AppSec [baseline] (32.231 ms) : 0, 32231
AppSec [candidate] (32.361 ms) : 0, 32361
Debugger [baseline] (68.303 ms) : 0, 68303
Debugger [candidate] (68.009 ms) : 0, 68009
Remote Config [baseline] (638.795 µs) : 0, 639
Remote Config [candidate] (650.887 µs) : 0, 651
Telemetry [baseline] (7.891 ms) : 0, 7891
Telemetry [candidate] (8.065 ms) : 0, 8065
Flare Poller [baseline] (3.754 ms) : 0, 3754
Flare Poller [candidate] (3.84 ms) : 0, 3840
ProfilingAgent [baseline] (96.996 ms) : 0, 96996
ProfilingAgent [candidate] (96.875 ms) : 0, 96875
Profiling [baseline] (97.572 ms) : 0, 97572
Profiling [candidate] (97.456 ms) : 0, 97456
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.55.0-SNAPSHOT~586c53ec38, baseline=1.56.0-SNAPSHOT~46649f7687
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.11 s) : 0, 1110119
Total [baseline] (8.887 s) : 0, 8886888
Agent [candidate] (1.113 s) : 0, 1112549
Total [candidate] (8.858 s) : 0, 8858221
section iast
Agent [baseline] (1.24 s) : 0, 1239911
Total [baseline] (9.555 s) : 0, 9555162
Agent [candidate] (1.25 s) : 0, 1250123
Total [candidate] (9.567 s) : 0, 9566668
gantt
title insecure-bank - break down per module: candidate=1.55.0-SNAPSHOT~586c53ec38, baseline=1.56.0-SNAPSHOT~46649f7687
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.484 ms) : 0, 1484
crashtracking [candidate] (1.46 ms) : 0, 1460
BytebuddyAgent [baseline] (710.725 ms) : 0, 710725
BytebuddyAgent [candidate] (711.975 ms) : 0, 711975
GlobalTracer [baseline] (249.775 ms) : 0, 249775
GlobalTracer [candidate] (250.049 ms) : 0, 250049
AppSec [baseline] (32.568 ms) : 0, 32568
AppSec [candidate] (32.662 ms) : 0, 32662
Debugger [baseline] (68.327 ms) : 0, 68327
Debugger [candidate] (68.8 ms) : 0, 68800
Remote Config [baseline] (638.896 µs) : 0, 639
Remote Config [candidate] (631.604 µs) : 0, 632
Telemetry [baseline] (8.119 ms) : 0, 8119
Telemetry [candidate] (8.328 ms) : 0, 8328
Flare Poller [baseline] (3.664 ms) : 0, 3664
Flare Poller [candidate] (3.818 ms) : 0, 3818
section iast
crashtracking [baseline] (1.446 ms) : 0, 1446
crashtracking [candidate] (1.466 ms) : 0, 1466
BytebuddyAgent [baseline] (829.059 ms) : 0, 829059
BytebuddyAgent [candidate] (836.109 ms) : 0, 836109
GlobalTracer [baseline] (237.245 ms) : 0, 237245
GlobalTracer [candidate] (238.981 ms) : 0, 238981
IAST [baseline] (26.494 ms) : 0, 26494
IAST [candidate] (29.332 ms) : 0, 29332
AppSec [baseline] (34.742 ms) : 0, 34742
AppSec [candidate] (32.407 ms) : 0, 32407
Debugger [baseline] (64.696 ms) : 0, 64696
Debugger [candidate] (65.287 ms) : 0, 65287
Remote Config [baseline] (530.89 µs) : 0, 531
Remote Config [candidate] (536.855 µs) : 0, 537
Telemetry [baseline] (7.542 ms) : 0, 7542
Telemetry [candidate] (7.732 ms) : 0, 7732
Flare Poller [baseline] (3.518 ms) : 0, 3518
Flare Poller [candidate] (3.53 ms) : 0, 3530
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 2 performance regressions! Performance is the same for 17 metrics, 16 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.55.0-SNAPSHOT~586c53ec38, baseline=1.56.0-SNAPSHOT~46649f7687
dateFormat X
axisFormat %s
section baseline
no_agent (1.185 ms) : 1173, 1196
. : milestone, 1185,
iast (3.158 ms) : 3123, 3193
. : milestone, 3158,
iast_FULL (5.685 ms) : 5629, 5741
. : milestone, 5685,
iast_GLOBAL (3.633 ms) : 3570, 3696
. : milestone, 3633,
profiling (1.986 ms) : 1969, 2003
. : milestone, 1986,
tracing (1.808 ms) : 1792, 1823
. : milestone, 1808,
section candidate
no_agent (1.174 ms) : 1163, 1186
. : milestone, 1174,
iast (3.367 ms) : 3326, 3408
. : milestone, 3367,
iast_FULL (5.619 ms) : 5562, 5676
. : milestone, 5619,
iast_GLOBAL (3.684 ms) : 3575, 3794
. : milestone, 3684,
profiling (2.045 ms) : 2027, 2063
. : milestone, 2045,
tracing (1.826 ms) : 1812, 1840
. : milestone, 1826,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.55.0-SNAPSHOT~586c53ec38, baseline=1.56.0-SNAPSHOT~46649f7687
dateFormat X
axisFormat %s
section baseline
no_agent (19.185 ms) : 18986, 19384
. : milestone, 19185,
appsec (18.308 ms) : 18122, 18493
. : milestone, 18308,
code_origins (17.978 ms) : 17799, 18156
. : milestone, 17978,
iast (17.327 ms) : 17156, 17499
. : milestone, 17327,
profiling (18.51 ms) : 18325, 18694
. : milestone, 18510,
tracing (17.682 ms) : 17506, 17857
. : milestone, 17682,
section candidate
no_agent (17.839 ms) : 17654, 18024
. : milestone, 17839,
appsec (18.435 ms) : 18247, 18622
. : milestone, 18435,
code_origins (17.608 ms) : 17434, 17782
. : milestone, 17608,
iast (17.702 ms) : 17524, 17881
. : milestone, 17702,
profiling (18.546 ms) : 18360, 18732
. : milestone, 18546,
tracing (17.74 ms) : 17563, 17917
. : milestone, 17740,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 2 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.55.0-SNAPSHOT~586c53ec38, baseline=1.56.0-SNAPSHOT~46649f7687
dateFormat X
axisFormat %s
section baseline
no_agent (14.977 s) : 14977000, 14977000
. : milestone, 14977000,
appsec (15.008 s) : 15008000, 15008000
. : milestone, 15008000,
iast (18.8 s) : 18800000, 18800000
. : milestone, 18800000,
iast_GLOBAL (17.708 s) : 17708000, 17708000
. : milestone, 17708000,
profiling (15.829 s) : 15829000, 15829000
. : milestone, 15829000,
tracing (14.55 s) : 14550000, 14550000
. : milestone, 14550000,
section candidate
no_agent (15.49 s) : 15490000, 15490000
. : milestone, 15490000,
appsec (14.854 s) : 14854000, 14854000
. : milestone, 14854000,
iast (18.102 s) : 18102000, 18102000
. : milestone, 18102000,
iast_GLOBAL (17.844 s) : 17844000, 17844000
. : milestone, 17844000,
profiling (14.979 s) : 14979000, 14979000
. : milestone, 14979000,
tracing (14.839 s) : 14839000, 14839000
. : milestone, 14839000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.55.0-SNAPSHOT~586c53ec38, baseline=1.56.0-SNAPSHOT~46649f7687
dateFormat X
axisFormat %s
section baseline
no_agent (1.472 ms) : 1461, 1484
. : milestone, 1472,
appsec (3.689 ms) : 3471, 3907
. : milestone, 3689,
iast (2.195 ms) : 2132, 2258
. : milestone, 2195,
iast_GLOBAL (2.244 ms) : 2180, 2307
. : milestone, 2244,
profiling (2.056 ms) : 2005, 2107
. : milestone, 2056,
tracing (2.021 ms) : 1972, 2070
. : milestone, 2021,
section candidate
no_agent (1.47 ms) : 1458, 1481
. : milestone, 1470,
appsec (3.75 ms) : 3528, 3972
. : milestone, 3750,
iast (2.202 ms) : 2139, 2266
. : milestone, 2202,
iast_GLOBAL (2.238 ms) : 2175, 2302
. : milestone, 2238,
profiling (2.512 ms) : 2338, 2686
. : milestone, 2512,
tracing (2.022 ms) : 1973, 2071
. : milestone, 2022,
|
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
| .replace(".", "_") | ||
|
|
||
| if (normalized !in supported && normalized !in aliasMapping) { | ||
| add("${file.name}:${i + 1} -> Config '$configValue' normalizes to '$normalized' which is not in supported-configurations.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to add another sentence explicitly recommending how to fix the error (and ~line 222). Perhaps something along the lines of "Please add the '$configValue' config to supported-configurations.json in or remove the config. More details in ."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, instead of supported-configurations.json, prefer the extension.jsonPath.get()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to add another sentence explicitly recommending how to fix the error (and ~line 222). Perhaps something along the lines of "Please add the '$configValue' config to supported-configurations.json in or remove the config. More details in ."
I added a more clear message in the Exception message. I'd rather not make the error logs too noisy by adding this detail for each missing config.
sarahchen6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
bric3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, few suggestions / polish though.
Walking the source code instead the java-parser lib can be used instead ? It's already in another of our plugin.
buildSrc/src/main/kotlin/datadog/gradle/plugin/config/ConfigInversionLinter.kt
Outdated
Show resolved
Hide resolved
buildSrc/src/main/kotlin/datadog/gradle/plugin/config/ConfigInversionLinter.kt
Outdated
Show resolved
Hide resolved
| .replace(".", "_") | ||
|
|
||
| if (normalized !in supported && normalized !in aliasMapping) { | ||
| add("${file.name}:${i + 1} -> Config '$configValue' normalizes to '$normalized' which is not in supported-configurations.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, instead of supported-configurations.json, prefer the extension.jsonPath.get()
bric3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, few nitpicks and suggestions. feel free to add apply them :)
buildSrc/src/main/kotlin/datadog/gradle/plugin/config/ConfigInversionLinter.kt
Outdated
Show resolved
Hide resolved
| val multiLineStartRegex = Regex("""public static final String (\w+) =$""") | ||
| // Multi-line value: `"value";` | ||
| val valueLineRegex = Regex(""""([^"]+)";""") | ||
| StaticJavaParser.setConfiguration(ParserConfiguration()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: I wonder if we should set explicitly the language level 🤔 to 1.8, when looking at the code of java parser, I see that the default is set to the POPULAR constant, however POPULAR appear to change regularly in minor versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to currently be set as Java 11, and was changed from Java 8 four years ago. But I agree that we can manually set it for consistency
| .filter { it is FieldDeclaration } | ||
| .map { it as FieldDeclaration } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I believe it's possible to avoid the filter operation by returning null in the mapping lambda, note the as? in kotlin which casts to given type or return null.
| .filter { it is FieldDeclaration } | |
| .map { it as FieldDeclaration } | |
| .map { it as? FieldDeclaration } |
| .orElse(null) | ||
|
|
||
| if (field != null && | ||
| field.hasModifiers(Modifier.Keyword.PUBLIC, Modifier.Keyword.STATIC, Modifier.Keyword.FINAL) && | ||
| varDecl.typeAsString == "String") { | ||
|
|
||
| val fieldName = varDecl.nameAsString | ||
| if (fieldName.endsWith("_DEFAULT")) return@forEach | ||
| val init = varDecl.initializer.orElse(null) ?: return@forEach | ||
|
|
||
| if (init !is StringLiteralExpr) return@forEach | ||
| val rawValue = init.value | ||
|
|
||
| val normalized = normalize(rawValue) | ||
| if (normalized !in supported && normalized !in aliasMapping) { | ||
| val line = varDecl.range.map { it.begin.line }.orElse(1) | ||
| add("$fileName:$line -> Config '$rawValue' normalizes to '$normalized' " + | ||
| "which is missing from '${extension.jsonFile.get()}'") | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: The orElse can be replaced by ifPresent { v -> }.
| .orElse(null) | |
| if (field != null && | |
| field.hasModifiers(Modifier.Keyword.PUBLIC, Modifier.Keyword.STATIC, Modifier.Keyword.FINAL) && | |
| varDecl.typeAsString == "String") { | |
| val fieldName = varDecl.nameAsString | |
| if (fieldName.endsWith("_DEFAULT")) return@forEach | |
| val init = varDecl.initializer.orElse(null) ?: return@forEach | |
| if (init !is StringLiteralExpr) return@forEach | |
| val rawValue = init.value | |
| val normalized = normalize(rawValue) | |
| if (normalized !in supported && normalized !in aliasMapping) { | |
| val line = varDecl.range.map { it.begin.line }.orElse(1) | |
| add("$fileName:$line -> Config '$rawValue' normalizes to '$normalized' " + | |
| "which is missing from '${extension.jsonFile.get()}'") | |
| } | |
| } | |
| .ifPresent { field -> | |
| if (field.hasModifiers(Modifier.Keyword.PUBLIC, Modifier.Keyword.STATIC, Modifier.Keyword.FINAL) && | |
| varDecl.typeAsString == "String") { | |
| val fieldName = varDecl.nameAsString | |
| if (fieldName.endsWith("_DEFAULT")) return@forEach | |
| val init = varDecl.initializer.orElse(null) ?: return@forEach | |
| if (init !is StringLiteralExpr) return@forEach | |
| val rawValue = init.value | |
| val normalized = normalize(rawValue) | |
| if (normalized !in supported && normalized !in aliasMapping) { | |
| val line = varDecl.range.map { it.begin.line }.orElse(1) | |
| add("$fileName:$line -> Config '$rawValue' normalizes to '$normalized' " + | |
| "which is missing from '${extension.jsonFile.get()}'") | |
| } | |
| } |
What Does This Do
This PR adds another Gradle task to the
ConfigInversionLinterplugin. The task goes through all Configuration definitions indd-trace-api/src/main/java/datadog/trace/api/configand ensures that all defined configurations exist in the localmetadata/supported-configurations.jsonfile. Note that some files (specificallyProfilingConfig.java) also define default values in the package, and the Gradle task accounts for that by skipping all strings ending with_DEFAULT.Additionally, the
ConfigInversionLinterplugin was mistakenly not activated in prior PRs. This PR activates the plugin to enable all relevant tasks.Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any useful labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]